[msbuild/dotnet] Use 'SdkIsSimulator' instead of 'ComputedPlatform'.#25234
[msbuild/dotnet] Use 'SdkIsSimulator' instead of 'ComputedPlatform'.#25234rolfbjarne wants to merge 7 commits intomainfrom
Conversation
This simplifies the code, since we only have a single property as the source of truth whether we're building for the simulator or not.
There was a problem hiding this comment.
Pull request overview
This PR replaces uses of $(ComputedPlatform) with $(SdkIsSimulator) across MSBuild and SDK-style targets to make simulator-vs-device detection rely on a single source of truth.
Changes:
- Updated multiple MSBuild target conditions to use
$(SdkIsSimulator)instead of$(ComputedPlatform). - Moved/added
ComputedPlatformdefinition intodotnet/targets/Xamarin.Shared.Sdk.propsfor compatibility with external consumers. - Removed the old
ComputedPlatforminitialization snippet fromXamarin.Shared.Sdk.DefaultItems.targets.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/monotouch-test/dotnet/shared.csproj | Uses SdkIsSimulator for defining AOT in test builds. |
| tests/ComputeRegistrarConstant.targets | Uses SdkIsSimulator to compute dynamic registrar constant for iOS/tvOS. |
| msbuild/Xamarin.iOS.Tasks.Windows/Xamarin.iOS.Common.After.targets | Uses SdkIsSimulator to gate IPA/dSYM copy-back steps from the Mac build host. |
| msbuild/Xamarin.Shared/Xamarin.iOS.Common.targets | Uses SdkIsSimulator to gate iTunes metadata/artwork targets. |
| msbuild/Xamarin.Shared/Xamarin.Shared.targets | Uses SdkIsSimulator to gate _PrepareResourceRules. |
| msbuild/Xamarin.Shared/Xamarin.Shared.props | Removes ComputedPlatform computation and switches certain defaults to SdkIsSimulator. |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Uses SdkIsSimulator for link-mode defaults previously keyed off ComputedPlatform. |
| dotnet/targets/Xamarin.Shared.Sdk.props | Exposes SdkIsSimulator and redefines ComputedPlatform for compatibility. |
| dotnet/targets/Xamarin.Shared.Sdk.DefaultItems.targets | Removes the previous ComputedPlatform initialization block. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Only set ComputedPlatform for iOS and tvOS (like the old behavior). Introduce a new SdkIsDevice property that's set to true when building for device (iOS and tvOS only), and use it to retain the original behavior of ComputedPlatform == 'iPhone' wherever needed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #1892441] Build passed (Build packages) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
✅ [PR Build #1892441] Build passed (Detect API changes) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ [CI Build #1892441] Build passed (Build macOS tests) ✅Pipeline on Agent |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
This PR aims to simplify simulator/device detection in the Apple-platform MSBuild tooling by replacing ComputedPlatform checks with SdkIsSimulator and a new SdkIsDevice property, establishing a single source of truth for “device vs simulator” decisions.
Changes:
- Added
SdkIsDeviceand updatedComputedPlatformcompatibility behavior in the .NET SDK props. - Replaced
ComputedPlatformconditions withSdkIsSimulator/SdkIsDeviceacross several targets and test build logic. - Documented the new
SdkIsDeviceMSBuild property and updated contributor guidance to check docs when introducing new properties.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/monotouch-test/dotnet/shared.csproj | Switches AOT define condition from ComputedPlatform to SdkIsDevice. |
| tests/ComputeRegistrarConstant.targets | Uses SdkIsSimulator instead of ComputedPlatform to decide dynamic registrar. |
| msbuild/Xamarin.iOS.Tasks.Windows/Xamarin.iOS.Common.After.targets | Replaces ComputedPlatform device gating for IPA/dSYM copy with SdkIsDevice. |
| msbuild/Xamarin.Shared/Xamarin.iOS.Common.targets | Gates iTunes metadata/artwork targets using SdkIsDevice instead of ComputedPlatform. |
| msbuild/Xamarin.Shared/Xamarin.Shared.targets | Updates _PrepareResourceRules condition to use SdkIsDevice. |
| msbuild/Xamarin.Shared/Xamarin.Shared.props | Removes ComputedPlatform computation and updates multiple conditions to use SdkIsSimulator/SdkIsDevice. |
| dotnet/targets/Xamarin.Shared.Sdk.targets | Updates library link-mode decisions to use SdkIsSimulator. |
| dotnet/targets/Xamarin.Shared.Sdk.props | Introduces SdkIsDevice and defines ComputedPlatform compatibility based on simulator/device. |
| dotnet/targets/Xamarin.Shared.Sdk.DefaultItems.targets | Removes the previous ComputedPlatform defaulting block. |
| docs/building-apps/build-properties.md | Adds documentation for SdkIsDevice. |
| .github/copilot-instructions.md | Adds guidance to update docs when introducing new MSBuild properties. |
| <!-- SdkIsDevice is true when building for device (which can only happen on iOS and tvOS). --> | ||
| <SdkIsDevice Condition="'$(SdkIsSimulator)' != 'true' And ('$(_PlatformName)' == 'iOS' Or '$(_PlatformName)' == 'tvOS')">true</SdkIsDevice> | ||
|
|
||
| <!-- We don't use the 'ComputedPlatform' property anymore, but it seems like there's code out there that depends on it (according to a search on GitHub at least), so define it like we did when we used the property. --> | ||
| <ComputedPlatform Condition="'$(ComputedPlatform)' == '' And '$(SdkIsSimulator)' == 'true'">iPhoneSimulator</ComputedPlatform> | ||
| <ComputedPlatform Condition="'$(ComputedPlatform)' == '' And '$(SdkIsDevice)' == 'true'">iPhone</ComputedPlatform> |
| <!-- We can archive: --> | ||
| <!-- macOS and Mac Catalyst: executable projects which aren't app extensions --> | ||
| <!-- iOS, tvOS and watchOS: executable projects built for device which aren't app extensions nor watch apps --> | ||
| <!-- iOS, tvOS and watchOS: executable projects built for device which aren't app extensions --> | ||
| <_CanArchive>false</_CanArchive> | ||
| <_CanArchive Condition="('$(_PlatformName)' == 'macOS' Or '$(_PlatformName)' == 'MacCatalyst') And '$(OutputType)' == 'Exe' And '$(IsAppExtension)' == 'false'">true</_CanArchive> | ||
| <_CanArchive Condition="('$(_PlatformName)' == 'iOS' Or '$(_PlatformName)' == 'tvOS' Or '$(_PlatformName)' == 'watchOS') And '$(OutputType)' == 'Exe' And '$(ComputedPlatform)' == 'iPhone' And '$(IsAppExtension)' == 'false' And '$(IsWatchApp)' == 'false'">true</_CanArchive> | ||
| <_CanArchive Condition="'$(OutputType)' == 'Exe' And '$(SdkIsDevice)' == 'true' And '$(IsAppExtension)' == 'false'">true</_CanArchive> | ||
|
|
| <_CanArchive>false</_CanArchive> | ||
| <_CanArchive Condition="('$(_PlatformName)' == 'macOS' Or '$(_PlatformName)' == 'MacCatalyst') And '$(OutputType)' == 'Exe' And '$(IsAppExtension)' == 'false'">true</_CanArchive> | ||
| <_CanArchive Condition="('$(_PlatformName)' == 'iOS' Or '$(_PlatformName)' == 'tvOS' Or '$(_PlatformName)' == 'watchOS') And '$(OutputType)' == 'Exe' And '$(ComputedPlatform)' == 'iPhone' And '$(IsAppExtension)' == 'false' And '$(IsWatchApp)' == 'false'">true</_CanArchive> | ||
| <_CanArchive Condition="'$(OutputType)' == 'Exe' And '$(SdkIsDevice)' == 'true' And '$(IsAppExtension)' == 'false'">true</_CanArchive> |
| @@ -153,7 +133,7 @@ Copyright (C) 2020 Microsoft. All rights reserved. | |||
| <!-- Disable dsymutil on desktop unless archiving --> | |||
| <NoDSymUtil Condition="'$(NoDSymUtil)' == '' And ('$(_PlatformName)' == 'macOS' Or '$(_PlatformName)' == 'MacCatalyst') And '$(ArchiveOnBuild)' != 'true'">true</NoDSymUtil> | |||
| <!-- Disable dsymutil for simulator builds by default --> | |||
| <NoDSymUtil Condition="'$(NoDSymUtil)' == '' And ('$(_PlatformName)' == 'iOS' Or '$(_PlatformName)' == 'tvOS' Or '$(_PlatformName)' == 'watchOS') And '$(ComputedPlatform)' != 'iPhone'">true</NoDSymUtil> | |||
| <NoDSymUtil Condition="'$(NoDSymUtil)' == '' And '$(SdkIsSimulator)' == 'true'">true</NoDSymUtil> | |||
| This property is a read-only property (setting it will have no effect) that | ||
| specifies whether we're building for a device or not. | ||
|
|
||
| This property is only `true` when building for an iOS or tvOS device (i.e., | ||
| when `SdkIsSimulator` is not `true` and the platform is iOS or tvOS). It is | ||
| not set for macOS or Mac Catalyst builds. |
🚀 [CI Build #1892441] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 156 tests passed 🎉 Tests counts✅ cecil: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Monterey (12): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
This simplifies the code, since we only have a single property as the source
of truth whether we're building for the simulator or not.